-
Notifications
You must be signed in to change notification settings - Fork 3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
FIx group chat confirmation list is displayed with delay #39364
FIx group chat confirmation list is displayed with delay #39364
Conversation
@Ollyws As you pointed out in the comment here, I also can't reproduce the blink. At first, I thought the issue But with/without |
@bernhardoj it does seem like on this PR the delay when displaying the currency selection is now alot longer: Current.movMain.mov |
That's really weird. Btw, I can see the "blink" now in this PR very briefly, the delay you and I see is most likely because the list scrolls to the target location and it's blank because not all items are rendered on the list. Screen.Recording.2024-04-03.at.18.51.40.movWeirdly though, the set state here somehow makes it faster.
I can put any set state and it makes it faster. Pretty sure that is an unintentional positive(?) effect. I think the safest way is to have a new prop (maybe call it |
Sounds good to me! |
@Ollyws updated |
Reviewer Checklist
Screenshots/VideosAndroid: Native01_Android_Native.mp4Android: mWeb Chrome02_Android_Chrome.mp4iOS: Native03_iOS_Native.mp4iOS: mWeb Safari04_iOS_Safari.mp4MacOS: Chrome / Safari05_MacOS_Chrome.mp4MacOS: Desktop06_MacOS_Desktop.mp4 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
@@ -297,6 +297,9 @@ type BaseSelectionListProps<TItem extends ListItem> = Partial<ChildrenProps> & { | |||
|
|||
/** Ref for textInput */ | |||
textInputRef?: MutableRefObject<TextInput | null>; | |||
|
|||
/** Whether to hide the list on initial render */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please update this comment to explain a little bit more why someone using the prop would choose true or false.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated. Please check.
@@ -297,6 +297,9 @@ type BaseSelectionListProps<TItem extends ListItem> = Partial<ChildrenProps> & { | |||
|
|||
/** Ref for textInput */ | |||
textInputRef?: MutableRefObject<TextInput | null>; | |||
|
|||
/** Whether to hide the list on the initial render. This would prevent the list from "blinking" when you have a long list and it auto scrolls to the bottom on mount but will delay the showing of the list */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is better thanks! Here is a suggestion that could improve it a little more:
When true, the list won't be visible until all items are rendered. This prevents the list from "blinking" as it's scrolled to the bottom which is recommended for large lists. When false, the list will render immediately and scroll to the bottom which works great for small lists.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great. Updated with a little modification.
-When true, the list won't be visible until all items are rendered.
+When true, the list won't be visible until the list layout is measured
because the isInitialSectionListRender
is set to false in onLayout
of the list
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Love it, thanks!
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
🚀 Deployed to production by https://github.com/Julesssss in version: 1.4.60-13 🚀
|
Details
The delay is because we set the opacity to 0 on initial render.
Fixed Issues
$ #39245
PROPOSAL: #39245 (comment)
Tests
Same as QA Steps
Offline tests
Same as QA Steps
QA Steps
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label and/or tagged@Expensify/design
so the design team can review the changes.ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Android: Native
Screen.Recording.2024-04-01.at.23.02.42.mov
Android: mWeb Chrome
Screen.Recording.2024-04-01.at.23.04.09.mov
iOS: Native
Screen.Recording.2024-04-01.at.23.03.13.mov
iOS: mWeb Safari
Screen.Recording.2024-04-01.at.23.08.44.mov
MacOS: Chrome / Safari
Screen.Recording.2024-04-01.at.23.00.30.mov
MacOS: Desktop
Screen.Recording.2024-04-01.at.23.02.19.mov